Skip to content

fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub - #1388

Closed
marcelo-maciel wants to merge 3 commits into
fullstackhero:mainfrom
marcelo-maciel:fix/minio-image-registry
Closed

marcelo-maciel wants to merge 3 commits into
fullstackhero:mainfrom
marcelo-maciel:fix/minio-image-registry

Conversation

@marcelo-maciel

@marcelo-maciel marcelo-maciel commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

MinIO withdrew minio/minio from Docker Hub. The repository is gone (GET https://hub.docker.com/v2/repositories/minio/minio/ answers {"message": "object not found"}), and a pull fails:

Error response from daemon: pull access denied for minio/minio, repository does not exist or may require 'docker login'

Everything in the kit that boots MinIO is broken on main today, with no change of ours involved:

  • Integration tests. The Testcontainers harness starts a MinIO container per fixture, so container start throws and every test in the project fails. On the current main that is Failed: 724, Passed: 23 in Integration.Tests, plus Integration.Middleware.Tests.
  • The Aspire AppHost (dotnet run --project src/Host/FSH.Starter.AppHost), the documented way to run the whole stack.
  • The Docker Compose deployment under deploy/docker/.

Fix

The image is still published, at quay.io/minio/minio. The registry moves, and the tag is pinned:

File Change
src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs MinioBuilder("quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z")
src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs same
src/Host/FSH.Starter.AppHost/AppHost.cs .WithImageRegistry("quay.io") + .WithImageTag(...)
deploy/docker/docker-compose.yml image: quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z
deploy/docker/README.md the service/image table

Why a pinned tag and not :latest. quay has not moved :latest since 2025-09-07, so both resolve to the same digest today and this changes nothing operationally. It removes the surprise of a silent move later, and keeps the test harness off a floating tag. Whether the kit should track a newer MinIO release, or a different S3-compatible image, is a separate call and not one to fold into an outage fix.

minio/mc (the minio-init one-shot in compose) is a different repository and still resolves on Docker Hub, so it is left alone.

While in the README's image table: the postgres and redis rows had drifted from what compose actually ships (postgres:18-alpine and valkey/valkey:9.1.0-alpine).

Also here: the two advisory fixes, or CI cannot go green

dotnet restore fails for the whole solution under TreatWarningsAsErrors on main right now, so this PR would be red regardless of the MinIO fix. Both are advisory-database drift rather than a code change: a commit green on 2026-08-10 is red today with no edits.

Verification

  • docker pull minio/minio:latest fails with the error quoted above.
  • docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z succeeds, digest sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e, the same digest :latest resolves to.
  • dotnet restore src/FSH.Starter.slnx exits 0 with no NU19xx.
  • dotnet build src/FSH.Starter.slnx -c Release -warnaserror: 0 warnings, 0 errors.
  • dotnet test src/Tests/Integration.Tests -c Release against a real MinIO container on the pinned tag: 0 failed, up from 23 passed / 724 failed before the change.
  • The Aspire manifest (--publisher manifest) renders the container as quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z, so the registry and tag overrides land where intended. minio/mc stays on Docker Hub, as expected.

Docs

Docs + changelog land in the separate fullstackhero/docs site: fullstackhero/docs#247.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@marcelo-maciel
marcelo-maciel force-pushed the fix/minio-image-registry branch from 672ce49 to a42d7f5 Compare September 14, 2026 16:29
@marcelo-maciel marcelo-maciel changed the title fix(infra): pull MinIO from quay.io, not Docker Hub fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub Sep 14, 2026
…advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as fullstackhero#1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.
MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.
The compose service and the Aspire init container still pointed at
minio/mc on Docker Hub, which is as gone as minio/minio: the Hub API
answers "object not found" for the repository. docker compose up died
in minio-init, and api/migrator never started behind their
service_completed_successfully gate; Aspire failed the same way through
WaitForCompletion.

Tag pinned to the newest release on quay.io (RELEASE.2025-08-13T08-35-41Z)
for the same reason the server image is pinned: quay stopped moving :latest.

Verified by running the path the original change never exercised:
docker compose up minio minio-init -> minio-init exits 0 with
"Bucket created successfully `local/fsh`"; dotnet build of the AppHost
exits 0.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

This one is independent of the rest of the batch and can merge as soon as it is reviewed: MinIO leaving Docker Hub breaks every Testcontainers-backed integration test at image-pull time, so several other open PRs carry a byte-identical copy of this fix just to reach their own code. Once this lands, those copies drop, but each of those branches needs update-branch first and will conflict in deploy/docker/README.md against this PR. The two sides resolve to the same content; it just will not happen on its own.

Full merge order for the batch, with the pairwise conflicts measured rather than guessed: #1369 comment.

marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 18, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.
@iammukeshm

Copy link
Copy Markdown
Member

Thanks @marcelo-maciel — this was the right fix when you opened it, but quay.io/minio/minio has since stopped allowing anonymous pulls (registry returns 401), so the pinned tag can't be pulled any more. Replaced MinIO with RustFS in #1390, which is now merged. The dependency bumps also landed separately (#1369, #1375), so your other PRs can drop both carried hunks on their next rebase.

@iammukeshm iammukeshm closed this Sep 25, 2026
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…ine (#1379)

* fix(web): honor X-Forwarded-* so the real client IP reaches the pipeline

UseHeroPlatform never called UseForwardedHeaders, so behind the reverse proxy
(Caddy / cloudflared) Connection.RemoteIpAddress was always the proxy container IP.
That collapsed the rate-limit partitions into a single install-wide bucket (one anonymous
spike throttles every tenant's login) and recorded a useless proxy IP on audit trails and
user sessions.

Register ForwardedHeadersOptions (X-Forwarded-For + X-Forwarded-Proto, known
networks/proxies cleared to trust the immediate upstream) and call UseForwardedHeaders
first in the pipeline, before HTTPS redirect / rate limiting / auth / audit read the client.
Lock the trusted set down via ForwardedHeadersOptions when the ingress topology is fixed.

* fix(web): bind forwarded-headers trust to configured proxies

Address review on #1334. Instead of clearing the known-proxy allow-list
(which trusts X-Forwarded-* from any source and reopens the IP-spoofing
hole this PR is meant to close), trust only the ingress proxies/networks
bound from the new TrustedProxyOptions, and honor a configurable
ForwardLimit for the real multi-hop ingress. With nothing configured the
framework default (loopback only) stands, so a client reaching the app
directly can't forge its IP/scheme.

Add a negative test proving an untrusted source's X-Forwarded-For is
ignored, alongside the trusted-proxy happy path. TestServer has no socket,
so the connection IP is stamped via a test-only startup filter.

* fix(web): name the offending setting when trusted-proxy config is malformed

A typo'd entry in TrustedProxyOptions surfaced as a bare FormatException from
IPAddress.Parse / IPNetwork.Parse, with nothing in the message pointing at the
setting that caused it. For config an operator edits once per deployment, under
time pressure, while wiring up an ingress, that is the wrong failure mode: the
silent version of it leaves the app trusting nobody while looking configured.

Both parses now use TryParse and throw an InvalidOperationException naming the
config path and the offending value.

Also closes two gaps the change exposed:

- TrustedProxyOptionsBindingTests pins the TrustedProxyOptions ->
  ForwardedHeadersOptions binding through AddHeroPlatform: the loopback-only
  default when the section is absent, KnownProxies + ForwardLimit binding, and
  both malformed-entry messages. Before this, renaming the config section broke
  nothing that any test could see. The host builder runs with DisableDefaults so
  an ambient TrustedProxyOptions__* on the machine cannot change what "nothing
  configured" resolves to.

- The untrusted-source integration test asserted only that the connection IP was
  persisted, which stays true when forwarded-header processing is absent
  entirely, so it passed with app.UseForwardedHeaders() removed. It now sends the
  identical header from the trusted proxy as well and asserts that arm is
  honored, so the trust boundary is what the test actually pins.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(web): rebuild the forwarded-headers trust list instead of appending to it

AddHeroPlatform only added to KnownProxies/KnownIPNetworks, which assumes whatever is
already there is the framework's loopback default. Under
ASPNETCORE_FORWARDEDHEADERS_ENABLED=true, ConfigureWebDefaults registers
ForwardedHeadersOptionsSetup, which empties both lists. An empty list is not "trust
nobody" in ForwardedHeadersMiddleware: it only validates the peer when at least one
entry exists, so the app rewrote RemoteIpAddress from an X-Forwarded-For sent by any
caller, forging the rate-limit partition and the audit IP.

Clear both lists unconditionally, then either restate the loopback default or apply the
configured proxies/networks.

The new test builds through WebApplication.CreateBuilder with the flag set, asserts
ForwardedHeadersOptionsSetup is actually registered so it cannot pass vacuously, and
checks the resolved lists equal a fresh ForwardedHeadersOptions.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* test(security): read the session this request created, not the newest one

`GetNewestSessionIpAsync` ordered `UserSessions` by `CreatedAt` and took the
first row from the whole table. It is safe only because the collection runs
serially; any other test in it issuing a token leaves the assertion reading a
row this request did not create. Ordering is not what makes it correct either:
`CreatedAt` comes from a single `TimeProvider.System` read and two issues can
land on the same tick, and `Id` is a random `Guid`, so a tiebreak on it picks
deterministically but not necessarily correctly.

Snapshot the session ids before the request and take the one that was not
there. `ShouldHaveSingleItem` asserts the correlation instead of assuming it.

Also states what the factory's `PostConfigure<ForwardedHeadersOptions>` leaves
these tests covering. It overwrites the flags, the forward limit and both trust
lists wholesale, so the `TrustedProxyOptions` binding is not what runs here -
the middleware and the placement of `UseForwardedHeaders` are. The binding has
its own gate in `Framework.Tests/Web/TrustedProxyOptionsBindingTests`, and the
comment now says so rather than reading as if this pinned production.

Verified: `dotnet test --filter FullyQualifiedName~ForwardedHeadersIpTests`
passes 2/2; inverting the new filter to `before.Contains(s.Id)` fails 2/2.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…1380)

* fix(mailing): send real HTML with a text alternative, not bare text

Every provider puts MailRequest.Body in the HTML slot — MailKit's
BodyBuilder.HtmlBody, SendGrid's htmlContent — but the password-reset and
welcome mails passed plain text. A bare URL inside an HTML part is not
auto-linked by most clients, so the reset link arrived as dead text and the
user had no way to complete the flow. The welcome mail additionally
interpolated the user-supplied first name straight into that HTML.

MailRequest gains an optional TextBody carrying the text/plain alternative.
SmtpMailService emits both parts as multipart/alternative; SendGridMailService
stops passing Body as plainTextContent, which had been shipping raw markup to
text-only clients. Identity builds its bodies through EmailBodies, which
HTML-encodes every interpolated value, and billing bodies gained their plain
twin so no message goes out HTML-only.

Verified: build -warnaserror 0/0; unit suites green (Identity 317,
Framework 122, Billing 123, and the rest).

* fix(deps): pin System.Security.Cryptography.Xml to 10.0.10

The test hosts pull 10.0.8 transitively, which carries HIGH-severity
advisories (GHSA-23rf-6693-g89p, GHSA-8q5v-6pqq-x66h, GHSA-cvvh-rhrc-wg4q,
GHSA-g8r8-53c2-pm3f) and trips NuGetAudit under TreatWarningsAsErrors,
breaking the build of every test project. 10.0.10 is the patched
servicing release. Mirrors the existing Microsoft.OpenApi transitive pin.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* fix(mailing): keep a text/plain part when only Body is supplied

Moving `plainTextContent` from `Body` to `TextBody` made the text part vanish
rather than become empty: `MailHelper.CreateSingleEmail` only adds it when the
string is non-null and non-empty. Every caller inside this repo was migrated, so
the tree is fine — but this is a template, and a consumer who still writes
`new MailRequest(to, subject, "Your code is 123456")` silently went from a
two-part message to HTML-only, with no compiler error and no warning.

`TextBody ?? Body` restores the old behaviour for them and changes nothing for a
caller that supplies both. Covered by a test that would have caught the drop.

* test(mailing): gate the body mapping on the default provider too

The `text/plain` regression this PR fixes was gated only on SendGrid, which is
the optional provider. SMTP is the default (`UseSendGrid` defaults to false), and
its body builder had no test at all, so putting the markup back into both parts
there would have kept the suite green.

The transport needs a server and is not where the mapping happens, so the MIME
build is what these drive: both parts present as `multipart/alternative`, HTML
alone staying `text/html` with no invented plain part, and an attachment wrapping
the alternative in `multipart/mixed` without dropping the text. `AddAttachmentsAsync`
is `internal` with `InternalsVisibleTo` for the test assembly, the same way
`Storage` already exposes its internals to the integration suite.

Mapping `TextBody` back to `request.Body` turns all three red.

* test(notifications): gate the module's mail bodies with its own test project

The module that produces every templated mail on this branch had no test
project at all, so the text/plain twin this PR is about was only gated at the
provider end. `Notifications.Tests` closes that: the templates are pure string
builders, so the whole thing runs without a DbContext or a host.

`BillingEmailBodiesTests` walks all four templates through one theory and
asserts the pair is a real pair - an HTML body carrying the subject and markup,
and a plain twin that is non-empty, repeats the subject and contains no angle
bracket. Plus the two places the copy can drift on its own: a tenant name is
escaped in the markup and left literal in the text part (entities in
text/plain are read as entities), and the optional due-date line disappears
without leaving a blank paragraph behind. `BillingEmailSenderTests` covers the
best-effort send: both bodies reach the `MailRequest`, an absent address sends
nothing, a throwing provider does not escape the integration-event handler, and
the warning it logs identifies the operation without carrying the recipient.

`IMailService` is ours, so the doubles are real implementations rather than
substitutes, per `.agents/rules/testing.md`. The module exposes its internals to
this assembly the same way `Mailing` already does for `Framework.Tests`.

Verified: `dotnet test src/Tests/Notifications.Tests` passes 13/13; making
`Expired` reuse its HTML as the text body fails the theory arm and the sender's
pass-through case. `dotnet build src/FSH.Starter.slnx` exits 0 with the project
registered, and Architecture.Tests stays 55/55.

* ci: run Notifications.Tests in the unit-test job

The job enumerates test projects by name rather than running the solution, so
adding `Notifications.Tests` to `FSH.Starter.slnx` put it in the build and
nowhere else: the previous run on this branch was green without executing a
single one of its tests. A gate CI never runs is not a gate.

Registering it in the slnx stays - that is what `dotnet test src/FSH.Starter.slnx`
and the IDE read - but this list is the wiring that matters on a PR.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…duplicates (#1378)

* fix(web): correct idempotent replay payload and serialize concurrent duplicates

Two defects in IdempotencyEndpointFilter:

API-01 — the filter cached JsonSerializer.SerializeToUtf8Bytes(result) where result
is the wrapped IResult (Ok<T>/Created<T>), so it stored {"value":...,"statusCode":200}
instead of the wire DTO, and it read Response.StatusCode before the IResult executed,
so a 201 Created replayed as 200. The handler result is now executed into a buffer to
capture the real wire body + status, which is what gets served and cached.

CONC-01 — probe->execute->write had no atomic reservation, so two concurrent requests
with the same key both missed the probe and both executed the handler. An atomic in-flight
reservation now serializes duplicates: Redis SET NX when an IConnectionMultiplexer is
registered (the multi-instance case — this stack already requires Redis there for the
shared Data Protection key ring), an in-process set otherwise (single instance). A
duplicate that arrives while the original is still running gets 409 Conflict.

Redis stays optional: without it the app falls back to the in-memory reservation, correct
for a single instance where a cross-container race cannot occur.

* fix(web): make idempotency replay actually engage (symmetric cache store)

The write went through HybridCache.SetAsync while the probe read IDistributedCache by
the raw key. HybridCache keys its L2 entries under its own scheme, so the probe never
found the entry and replay silently never engaged — even in production. Proven by
un-skipping ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused,
which now passes.

Write to the same IDistributedCache, key and serializer the probe uses. Idempotency
entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping
HybridCache here loses nothing.

* fix(web): short-TTL, fail-open idempotency reservation

Address review on #1333:

- Reservation used the 24h response TTL, so a crash between reserving
  and the finally-release stranded the Redis lock for a day (every retry
  409s). Add IdempotencyOptions.ReservationTtl (default 1m), decoupled
  from DefaultTtl.
- Reserve now fails open on a transient Redis error instead of 500ing
  the request, matching the best-effort stance of the response write.
- Guard the release KeyDeleteAsync so a Redis fault can't throw out of
  the finally.

Tests: reservation uses ReservationTtl not DefaultTtl; a faulting Redis
on reserve/release proceeds without throwing (exercises the Redis NX
branch the prior tests skipped).

* fix(web): make the stored idempotent response outlive the request

Three defects surfaced by re-reading the whole filter rather than the delta.

The response store was tied to the client's connection, so the retry that
idempotency exists to serve re-executed the handler. Two paths caused it, not
one: the body write to the client ran before the store, so a closed socket threw
and skipped it entirely; and the capture itself ran under RequestAborted, where
WriteAsJsonAsync swallows the cancellation and hands back an EMPTY body — which
was then cached and replayed as a 200 for the full 24h TTL. The capture now runs
with the abort token detached (it writes to an in-memory buffer, never the
socket), the store runs before the client write and on CancellationToken.None,
and only then does the body go out.

Replay dropped every response header, so a replayed 201 arrived without
Location: a client that follows the header worked on the first call and broke on
the retry. The captured response now carries an allow-listed set (Location,
ETag) and replays it. Transport and host-owned headers stay out — a stale
Content-Length would corrupt the response.

Non-2xx is no longer stored. Faithful status capture made the pre-existing
behaviour bite: a transient downstream failure locked the caller out of that key
for 24h. A failure is not a record of a committed side effect.

CachedIdempotentResponse is no longer a HybridCache type, so its
[ImmutableObject(true)] contract (and the CachedTypeContractTests entry
asserting it) described a store this filter stopped using. Both dropped. The new
Headers property defaults to empty so entries written before it deserialize.

Tests: replayed 201 carries Location; a first call whose client disconnects
still replays the real DTO body; a non-2xx first response lets the retry run.
All three fail on the previous commit and pass here.

* fix(web): scope the idempotency entry to the operation, not the tenant alone

The entry was keyed on tenant + caller key, with nothing identifying the
operation. One key reused against a second idempotent endpoint replayed the
first endpoint's response and the second request silently never ran. Thirty-one
endpoints across eight modules share that namespace, and one of them
(self-registration) is anonymous: it resolves no tenant claim, so every caller
of it lands in the same "global" bucket. This was latent only for as long as
replay never engaged — the fix that makes replay work is what would have put it
on the wire. The key now folds in the HTTP method and the route pattern.

Two smaller things in the same area. The 409 for an in-flight duplicate and the
400 for an over-long key emitted a bare JSON string, where every other error
these endpoints produce is RFC 9457 ProblemDetails; both now match. And an
unreadable cache entry (written by another version, or another writer at the
same key) let JsonException escape as a 500 — that path only became reachable
once replay started engaging at all. It now degrades to a miss and logs.

ReleaseReservationAsync also swallows cancellation now, not just faults: it runs
in a finally after the response body has already gone to the client, so anything
thrown there can only reset the connection on a request that succeeded.

Tests: a key reused across two route patterns runs the second handler; an
unreadable entry runs the handler (with a valid entry seeded at the same key
first, so the assertion can't pass as a plain cache miss); a 204 replays without
a fabricated content type. Each fails on a mutated implementation.

* fix(web): close the idempotency reservation's races

The reservation guarded the handler against concurrent duplicates but four
holes let one through anyway, or locked a caller out of a key:

- The entry was keyed on the caller's `tenant` claim. A root operator scoping
  requests to different tenants shares one "root" bucket, so one key reused
  across two targets replays the first tenant's body to the second. Key off the
  resolved tenant context instead — the one BaseDbContext scopes the side effect
  to — with the claim as the fallback for a JWT-only request (Finbuckle's claim
  strategy runs pre-authentication and resolves nothing for those). The raw
  `tenant` header is deliberately not a fallback: an unresolved header is one
  Finbuckle refused, and an unvalidated value has no business in a shared key.

- The cache was probed once, before the reservation. The original request can
  store its response and release the lock inside that window; the duplicate then
  takes the free lock and executes the handler again. Probe once more with the
  lock held.

- The lock was a `:inflight` suffix on the entry key, so a caller key ending in
  that suffix put its 24h entry exactly where another key's lock goes — every
  later request with that key 409s for the full response TTL. Give the lock its
  own prefix.

- Release was an unconditional delete. A request that failed open on a Redis
  blip, or one whose reservation had already expired, freed a lock another
  request was holding. Release via compare-and-delete against the token the
  reservation was taken with; failing open carries no token and deletes nothing.

The in-process fallback also gains the TTL takeover the Redis branch gets for
free: without it a handler that never returns strands the key until the process
restarts and every retry 409s forever.

Each fix is pinned by a test that was verified to fail when the fix is reverted.

* fix(web): keep an idempotent handler alive past a client disconnect

Follow-up from adversarial passes over the whole filter. Each item below is
pinned by a test verified to fail when the fix is reverted.

- The handler ran under the client's abort token. A disconnect after the side
  effect committed cancelled the next await inside the handler (an EF read, an
  outbox write, a Mediator behaviour), so the filter had nothing to store and
  the client's retry re-executed the side effect — the duplicate this filter
  exists to absorb. The handler now runs with the token detached; the trade is
  that a disconnect no longer aborts an idempotent handler.

- The probe was the one link that hard-failed. Reserve and store both degrade to
  a warning when the cache is down, so a `RedisConnectionException` on the probe
  took every idempotent endpoint down for exactly the clients that send a key.
  It now fails open as a miss.

- A handler that writes the response itself had already started it, so the buffer
  swap captured nothing and setting the captured status threw. That case now
  passes through untouched and stores nothing.

- The key covered the route pattern but not its values, so `PUT /tickets/1` and
  `PUT /tickets/2` were one operation: the second replayed the first ticket's
  response and never ran. It now folds in the resolved route values.

- The key was not scoped to the caller, so two users of one tenant reusing a
  low-entropy key on the same endpoint received each other's response bodies
  while their own request was silently suppressed.

- The 409 said "retry shortly" with no `Retry-After`. It now sends 1 second: the
  original is normally about to store its response, and the reservation TTL is
  the worst case, not the hint.

Also: options are validated at startup like every other block here (a zero TTL
failed silently inside the best-effort write, so nothing was ever stored),
`CacheKeys.Tags.Idempotency` no longer claims to be applied, and the cached
headers dictionary documents that its comparer does not survive deserialization.
Ceilings that stay: no size cap on the buffered response (do not put
`.WithIdempotency()` on a streaming endpoint), no lease renewal, and a lock whose
Redis may not be the cache's Redis — all three now carry `ponytail:` notes.

* test(web): cover the idempotency branches no test could fail on

A test-quality pass over the suite found assertions that survive the mutation
they exist to catch, and branches with no test at all. Each case below now fails
when the behaviour it pins is reverted.

Assertions that could not fail:
- The Lua release script was matched with Arg.Any<string>() while the fake
  hardcoded compare-and-delete, so swapping the script for an unconditional
  `del` kept the suite green — the exact bug the script's comment warns about.
  The script text is asserted now.
- `(result as IStatusCodeHttpResult)?.StatusCode.ShouldBe(409)` skips the whole
  assertion for a result that isn't one, which is precisely the mutation it
  guards. Cast instead.
- The concurrency test relied on the default one-minute ReservationTtl outliving
  the test; a CI freeze past it hands the key over and fails a correct filter.
  It pins the TTL explicitly.

Branches with no coverage: a handler that throws (the release has to stay in the
finally, or one exception strands the key until the TTL), the tenant-claim
fallback (collapsing it to "global" puts every JWT-only caller in one bucket and
replays across tenants), the refused duplicate's re-probe, the restrictive half
of the header allow-list (Set-Cookie must not come back on a replay), the
best-effort store, a faulting release, the no-header pass-through, the
MaxKeyLength rejection, and an entry stored without the Headers member — the
shape a previous version wrote, which has to keep replaying through a rolling
deploy.

Also drops a stale comment claiming body capture is out of reach; this PR is
what made it possible, and the integration suite asserts it end to end.

* test(identity): pin that the caller id claim survives bearer inbound mapping

The idempotency filter scopes its cache key by ClaimsPrincipal.GetUserId(), which
reads ClaimTypes.NameIdentifier only. Until now nothing proved that claim type is
present after JwtBearer validates a real issued token: every existing test built the
principal by hand, so caller scoping could have been inert in production (every
caller collapsing into one bucket) with a green suite.

Round-trips a token from TokenService through JsonWebTokenHandler configured with
JwtBearerOptions' own MapInboundClaims default, then asserts GetUserId() resolves.
Verified with the claim removed from the token as well: the short-form `sub` maps to
it, so both shapes IdentityService emits resolve.

* docs(agents): idempotency rule covers the abort-token, probe and key changes

The rule described the reservation work from the previous round but not what
landed after it, so an agent reading it would still believe the probe hard-fails
and the key ignores route values and the caller. Adds the abort-token detachment
together with the constraint it implies (no streaming endpoints), the HasStarted
pass-through, Retry-After and the startup validation.

* test(web): assert idempotency options through IStartupValidator, not lazily

The startup-validation tests resolved IOptions<IdempotencyOptions>.Value, which
validates on first access with or without .ValidateOnStart(). Deleting that call
left all six green while moving the failure from boot to the first request that
carries an Idempotency-Key — a suite that could not see the difference between
"rejected at startup" and "rejected once, in production, per process".

They now go through IStartupValidator, which is what .ValidateOnStart() registers
and what the host runs before serving traffic. Verified: with .ValidateOnStart()
removed, 6 of the 10 tests fail.

* test(web): pin each idempotency options clause to its own failure message

Asserting only OptionsValidationException let a clause be deleted with every row
still green: a zero DefaultTtl also trips "ReservationTtl must not exceed
DefaultTtl", so the row aimed at DefaultTtl passed on the wrong clause. Each row
now names the failure it expects.

Per-clause mutation run: removing any one of the five clauses fails exactly the
one row that targets it; removing .ValidateOnStart() fails all six cases, since
IStartupValidator is then unregistered.

* fix(identity): drop idempotency from self-registration, and gate anonymous endpoints

The cache key scopes by caller, and ResolveCaller returns "anon" for every
unauthenticated request, so on an anonymous endpoint all callers share one bucket.
Two people registering on the same tenant with the same low-entropy key ("1",
"retry") built the identical key: the second replayed the first registrant's 201
with the first registrant's UserId, and their own account was silently never
created. /self-register was the only anonymous idempotent endpoint. It was
unreachable until replay started engaging.

A retry there is already safe without the filter — the unique-email constraint
rejects the duplicate — so the endpoint drops .WithIdempotency() rather than
gaining a body fingerprint.

Deleting one call would leave nothing stopping the next one, so WithIdempotency()
now attaches IdempotentEndpointMetadata: an endpoint filter is invisible in
metadata, and the marker makes the wiring inspectable. IdempotencyWiringTests
walks the endpoint map and fails when an AllowAnonymous() endpoint carries it,
plus a second test that fails if the marker stops being attached, so the first
cannot pass over an empty set.

* test(multitenancy): carry the response body into the theme status assertions

UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTargetsTenantA was seen
returning 401 instead of 204 twice on a loaded machine, then passed four runs in a
row (including two with 14 of 16 cores saturated) and passes in isolation and in
CI. A bare status assertion gives nothing to work with: the reason JwtBearer
rejected the token is in the ProblemDetails body, which the test discarded.

The assertions now report method, URL and body on mismatch. Exercised by
expecting the wrong status on purpose: the failure message carries the body.

This is diagnosis, not a fix. The cause is still unidentified, and this test can
still go red.

* build(deps): pin SSH.NET to the patched 2026.0.0

The Testcontainers packages pull SSH.NET 2025.1.0 transitively, which carries
GHSA-q939-rpr3-3284 (CVE-2026-48798, high): ScpClient recursive download writes
files outside the target directory. Under TreatWarningsAsErrors that advisory is
NU1903 as an error, so `dotnet restore src/FSH.Starter.slnx` fails for the whole
solution — Backend CI, CodeQL and the template smoke build all die at restore.

Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping
Testcontainers does not clear it. 2026.0.0 is the first patched release, and
transitive pinning is already enabled, so this entry alone bumps it — same shape
as the MessagePack, Microsoft.OpenApi and SQLitePCLRaw pins next to it.

* fix(web): keep request-derived text out of the idempotency warning

CodeQL flagged the `Response.HasStarted` pass-through warning (alert 28,
cs/log-forging): it logged `operation`, which folds in `Request.Method`, the
resolved route values and the raw request path, so three caller-controlled
sources reached a log line. Every other log in the filter already passes
`HashKey(...)`, which is why this was the only one.

The warning now logs the route pattern read off the endpoint's `RoutePattern`,
a literal from the route table, which identifies the endpoint just as well.
`operation` is unchanged for the cache key, where the route values have to stay:
`PUT /tickets/1` and `PUT /tickets/2` are different operations.

* fix(web): detach the bound CancellationToken argument, not only RequestAborted

Minimal-API parameter binding resolves a handler's CancellationToken from
HttpContext.RequestAborted before endpoint filters run, so the handler already
holds a copy of the original token. Reassigning the property inside the filter
never reached it: a client disconnect after the side effect committed could
still cancel an await inside the handler, leaving the filter with nothing to
store and letting the retry execute the side effect a second time.

Replace the bound argument as well. RequestAborted keeps being reassigned for
code that reads it directly instead of taking it as a parameter.

The regression test runs a real host over the real filter; a hand-built
EndpointFilterInvocationContext skips binding entirely and cannot see this.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* test(idempotency): exercise the Redis refusal, not just the in-process one

Every concurrency test ran with `multiplexer: null`, so the branch that actually
refuses a duplicate in a multi-instance deployment — `StringSetAsync(...,
When.NotExists)` coming back false — was never executed. The one test that does
share a keyspace asserts the opposite case (a key that must NOT be blocked).

Verified by mutation: flipping the reservation to `When.Always` turns the new
test red and leaves the rest of the suite green, which is exactly the regression
that used to be invisible.

* fix(idempotency): let an endpoint cap its own replay window

`RequestUploadUrl` returns a presigned URL good for `FilesOptions.UploadUrlTtlMinutes`
(15 by default) and cached the response for `DefaultTtl`, 24 hours. A client
retrying with the same key an hour later got a 200 carrying a URL that had
expired 45 minutes earlier, with no way out except inventing a new key. Dropping
`.WithIdempotency()` from the endpoint was the other option and is worse: the
handler INSERTs a pending `FileAsset` and pre-checks quota, so the common case (a
network retry seconds later) would start duplicating rows to fix the rare one.

`WithIdempotency(TimeSpan)` puts the window on the endpoint that knows it.
`IdempotentEndpointMetadata` carries it, the filter prefers it over the
configured default, and the endpoint reads the value at map time from the same
option the handler mints the URL with, so the two cannot drift. A non-positive
TTL throws rather than expiring every entry on write, which would leave the
endpoint advertising an idempotency it no longer has.

Two more from the same review:

- A handler returning a bare `string` was captured through `WriteAsJsonAsync`,
  which quotes it and sends `application/json`. Minimal APIs write `text/plain`,
  so the first response through this filter differed from the same handler's
  response without it, and the replay then repeated the difference.
- The filter comment and `.agents/rules/security.md` both described anonymous
  endpoints as a live case of the shared bucket while `IdempotencyWiringTests`
  fails the build for exactly that. They now describe the floor the code has,
  not a configuration it forbids.

* test(idempotency): make the client-abort test actually abort

`IdempotentHandler_Should_RunToCompletion_When_ClientAbortIsSignalled` signalled
nothing: the handler slept 20ms on its own token and the assertion passed whether
or not the filter detached anything. Deleting `DetachBoundCancellationTokens` left
it green.

The abort is real now. A middleware ahead of the endpoint publishes a token the
test controls as `RequestAborted` (TestServer cannot hang up a live request from
the client side), and the handler cancels it mid-flight. The first response then
never reaches the client, which is correct and is asserted rather than swallowed:
the store is deliberately sequenced ahead of the body write for this case. What
the test asserts is the point of the detach, that the retry replays instead of
committing the side effect twice.

With the detach reverted, both tests in the class go red.

* docs(agents): condense the idempotency rule into scannable bullets

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
* fix(web): honor X-Forwarded-* so the real client IP reaches the pipeline

UseHeroPlatform never called UseForwardedHeaders, so behind the reverse proxy
(Caddy / cloudflared) Connection.RemoteIpAddress was always the proxy container IP.
That collapsed the rate-limit partitions into a single install-wide bucket (one anonymous
spike throttles every tenant's login) and recorded a useless proxy IP on audit trails and
user sessions.

Register ForwardedHeadersOptions (X-Forwarded-For + X-Forwarded-Proto, known
networks/proxies cleared to trust the immediate upstream) and call UseForwardedHeaders
first in the pipeline, before HTTPS redirect / rate limiting / auth / audit read the client.
Lock the trusted set down via ForwardedHeadersOptions when the ingress topology is fixed.

* fix(web): bind forwarded-headers trust to configured proxies

Address review on #1334. Instead of clearing the known-proxy allow-list
(which trusts X-Forwarded-* from any source and reopens the IP-spoofing
hole this PR is meant to close), trust only the ingress proxies/networks
bound from the new TrustedProxyOptions, and honor a configurable
ForwardLimit for the real multi-hop ingress. With nothing configured the
framework default (loopback only) stands, so a client reaching the app
directly can't forge its IP/scheme.

Add a negative test proving an untrusted source's X-Forwarded-For is
ignored, alongside the trusted-proxy happy path. TestServer has no socket,
so the connection IP is stamped via a test-only startup filter.

* fix(web): name the offending setting when trusted-proxy config is malformed

A typo'd entry in TrustedProxyOptions surfaced as a bare FormatException from
IPAddress.Parse / IPNetwork.Parse, with nothing in the message pointing at the
setting that caused it. For config an operator edits once per deployment, under
time pressure, while wiring up an ingress, that is the wrong failure mode: the
silent version of it leaves the app trusting nobody while looking configured.

Both parses now use TryParse and throw an InvalidOperationException naming the
config path and the offending value.

Also closes two gaps the change exposed:

- TrustedProxyOptionsBindingTests pins the TrustedProxyOptions ->
  ForwardedHeadersOptions binding through AddHeroPlatform: the loopback-only
  default when the section is absent, KnownProxies + ForwardLimit binding, and
  both malformed-entry messages. Before this, renaming the config section broke
  nothing that any test could see. The host builder runs with DisableDefaults so
  an ambient TrustedProxyOptions__* on the machine cannot change what "nothing
  configured" resolves to.

- The untrusted-source integration test asserted only that the connection IP was
  persisted, which stays true when forwarded-header processing is absent
  entirely, so it passed with app.UseForwardedHeaders() removed. It now sends the
  identical header from the trusted proxy as well and asserts that arm is
  honored, so the trust boundary is what the test actually pins.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* fix(web): reject a trusted-proxy ForwardLimit below 1 at startup

TrustedProxyOptions.ForwardLimit was passed straight to ForwardedHeadersOptions
with no validation, and neither bad value announces itself. Zero truncates the
unwind loop in ApplyForwarders to zero iterations, so X-Forwarded-* stop being
processed with no error and no log while the config still reads as configured.
A negative value makes the middleware allocate a negative-length buffer, which
throws OverflowException on every request - including requests carrying no
forwarded headers at all - and UseForwardedHeaders sits after UseExceptionHandler,
so that surfaces as a plain 500 rather than a boot failure a smoke test catches.

Reject anything below 1 where the malformed KnownProxies/KnownNetworks entries
are already rejected, naming the setting and the offending value. The throw lands
during startup, so a bad hop count fails the deploy instead of the traffic.

Closes #1358

* docs(web): record why X-Forwarded-Host stays out of the flag list

Review note from #1334, left for the follow-up: the flag list carries only
X-Forwarded-For and X-Forwarded-Proto, and the omission is deliberate. Rewriting
Request.Host from a header is a host-header injection primitive, and the three
Identity endpoints that build a public URL from the request would then mail
confirmation links pointing wherever the header said. The consequence an operator
has to know is that Request.Host keeps the internal host behind a proxy, and those
links carry it.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(web): rebuild the forwarded-headers trust list instead of appending to it

AddHeroPlatform only added to KnownProxies/KnownIPNetworks, which assumes whatever is
already there is the framework's loopback default. Under
ASPNETCORE_FORWARDEDHEADERS_ENABLED=true, ConfigureWebDefaults registers
ForwardedHeadersOptionsSetup, which empties both lists. An empty list is not "trust
nobody" in ForwardedHeadersMiddleware: it only validates the peer when at least one
entry exists, so the app rewrote RemoteIpAddress from an X-Forwarded-For sent by any
caller, forging the rate-limit partition and the audit IP.

Clear both lists unconditionally, then either restate the loopback default or apply the
configured proxies/networks.

The new test builds through WebApplication.CreateBuilder with the flag set, asserts
ForwardedHeadersOptionsSetup is actually registered so it cannot pass vacuously, and
checks the resolved lists equal a fresh ForwardedHeadersOptions.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* docs(web): say what a ForwardLimit above the real hop count costs

The option documented the failure of setting it too low and the two invalid
values, but not the one an operator is most likely to reach for: rounding it up
"to be safe". The middleware trusts one entry per hop counting from the right and
only ever checks the peer, so a limit of 2 behind a single proxy hands the caller
its own RemoteIpAddress, and every IP-based rate limit and audit entry follows it.

* test(security): read the session this request created, not the newest one

`GetNewestSessionIpAsync` ordered `UserSessions` by `CreatedAt` and took the
first row from the whole table. It is safe only because the collection runs
serially; any other test in it issuing a token leaves the assertion reading a
row this request did not create. Ordering is not what makes it correct either:
`CreatedAt` comes from a single `TimeProvider.System` read and two issues can
land on the same tick, and `Id` is a random `Guid`, so a tiebreak on it picks
deterministically but not necessarily correctly.

Snapshot the session ids before the request and take the one that was not
there. `ShouldHaveSingleItem` asserts the correlation instead of assuming it.

Also states what the factory's `PostConfigure<ForwardedHeadersOptions>` leaves
these tests covering. It overwrites the flags, the forward limit and both trust
lists wholesale, so the `TrustedProxyOptions` binding is not what runs here -
the middleware and the placement of `UseForwardedHeaders` are. The binding has
its own gate in `Framework.Tests/Web/TrustedProxyOptionsBindingTests`, and the
comment now says so rather than reading as if this pinned production.

Verified: `dotnet test --filter FullyQualifiedName~ForwardedHeadersIpTests`
passes 2/2; inverting the new filter to `before.Contains(s.Id)` fails 2/2.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…1385)

* fix(mailing): send real HTML with a text alternative, not bare text

Every provider puts MailRequest.Body in the HTML slot — MailKit's
BodyBuilder.HtmlBody, SendGrid's htmlContent — but the password-reset and
welcome mails passed plain text. A bare URL inside an HTML part is not
auto-linked by most clients, so the reset link arrived as dead text and the
user had no way to complete the flow. The welcome mail additionally
interpolated the user-supplied first name straight into that HTML.

MailRequest gains an optional TextBody carrying the text/plain alternative.
SmtpMailService emits both parts as multipart/alternative; SendGridMailService
stops passing Body as plainTextContent, which had been shipping raw markup to
text-only clients. Identity builds its bodies through EmailBodies, which
HTML-encodes every interpolated value, and billing bodies gained their plain
twin so no message goes out HTML-only.

Verified: build -warnaserror 0/0; unit suites green (Identity 317,
Framework 122, Billing 123, and the rest).

* fix(deps): pin System.Security.Cryptography.Xml to 10.0.10

The test hosts pull 10.0.8 transitively, which carries HIGH-severity
advisories (GHSA-23rf-6693-g89p, GHSA-8q5v-6pqq-x66h, GHSA-cvvh-rhrc-wg4q,
GHSA-g8r8-53c2-pm3f) and trips NuGetAudit under TreatWarningsAsErrors,
breaking the build of every test project. 10.0.10 is the patched
servicing release. Mirrors the existing Microsoft.OpenApi transitive pin.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* refactor(mailing): one HTML shell and one encoder for every module

Follow-up to the nit on #1351: `EmailBodies` (Identity) and
`BillingEmailBodies.Wrap` (Notifications) had grown into two independent HTML
shells with two different escapers, and they would have drifted.

- New `FSH.Framework.Mailing.HtmlEmail` holds the document shell (doctype,
  charset, viewport, card) and the encoder. Both modules already referenced the
  Mailing building block, so no new project reference.
- `Encode` is `WebUtility.HtmlEncode` everywhere. The hand-rolled four-`Replace`
  chain in Notifications covered only `&`, `<` and `>` — safe in element content,
  not in an attribute — and is gone.
- `EmailBodies` is deleted; its two callers use `HtmlEmail` directly rather than
  a pass-through.
- Billing mail now renders in the same document as identity mail, so it gains a
  doctype and a `<meta charset>` it did not have.

`Shell` takes trusted markup and does not encode it; the doc comment says so and
a test pins it, because "hardening" that would render every e-mail as visible
tags.

* fix(notifications): encode the invoice amount, currency included, in the HTML part

`amountText` embeds `currency`, which is data rather than a literal, and was the
only value in this file reaching the markup unencoded — `invoiceNumber`,
`tenantName` and `plan` were all escaped already.

Not a vulnerability today, and the description says so: the only writer is
`CreatePlanCommand`, capped at three characters by its validator and gated by
`BillingPermissions.Manage`, while the top-up path passes a hardcoded "USD".
Three characters in element content cannot form a working payload. This is
consistency and defence in depth: the only thing standing between the value and
the markup is a length rule in another module.

No-op for every real currency code: encoding "100.00 USD" returns it unchanged.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* fix(mailing): keep a text/plain part when only Body is supplied

Moving `plainTextContent` from `Body` to `TextBody` made the text part vanish
rather than become empty: `MailHelper.CreateSingleEmail` only adds it when the
string is non-null and non-empty. Every caller inside this repo was migrated, so
the tree is fine — but this is a template, and a consumer who still writes
`new MailRequest(to, subject, "Your code is 123456")` silently went from a
two-part message to HTML-only, with no compiler error and no warning.

`TextBody ?? Body` restores the old behaviour for them and changes nothing for a
caller that supplies both. Covered by a test that would have caught the drop.

* refactor(mailing): fix a misleading test name and two dead lines

The encoder test was named Encode_Should_EncodeOnce_When_ValueIsAlreadyEscaped
and commented as guarding against double-encoding, while asserting exactly that
double-encoding. The behaviour is right — the input is text, so a literal
ampersand must be escaped even when it spells an entity — but the name invited
someone to 'fix' the encoder into leaving raw ampersands in markup.

Also drops an orphan using (EmailBodies is deleted) and the two blank entries in
the text builder that the Join filtered straight back out.

* docs(mailing): say plainly which half of the claim is true

The type doc said HtmlEmail was the single shell for outbound mail. It is the
single encoder; UserRegistrationService.BuildConfirmationEmailHtml still builds
its own document, and that is the most-seen e-mail in the product. Migrating it
changes layout and copy, so it belongs in its own PR — but the doc should not
claim coverage that does not exist in the meantime.

* refactor(identity): drop EmailBodies, superseded by the shared HtmlEmail

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…inks (#1377)

* fix(identity): resolve front-end origin per-request for auth e-mail links

Password-reset and e-mail-confirmation links were built from a single
configured OriginUrl (which pointed at the API and was empty in
Production, throwing "Origin URL is not configured") or from the raw
request host (the API), so neither could target the correct SPA when
more than one front-end is served (admin :5173, dashboard :5174).

Introduce IOriginResolver:
- FrontendOrigin(): takes the request Origin header and validates it
  against CorsOptions.AllowedOrigins, so the reset/confirmation link
  lands on the SPA the request came from. The allow-list check is the
  security boundary: a forged Origin on the anonymous forgot-password
  flow can never be injected into an e-mail. Throws when no allow-listed
  origin is present.
- ApiOrigin(): configured origin, else request host (unchanged
  behaviour) for API-served assets (avatars) and RequestContextService.

The confirmation e-mail now points at the SPA `/confirm-email` page
(which already exists in both clients and calls the API) instead of the
API route directly.

- forgot-password, register, self-register and resend-confirmation now
  resolve the front-end origin via the resolver.
- avatar URL building and RequestContextService delegate to ApiOrigin().
- appsettings: add the dev SPA origins to CorsOptions.AllowedOrigins.
  Production deployments must list their SPA URLs there.
- tests: OriginResolverTests (allow-list, case/slash/port, forged origin,
  missing header), updated ForgotPassword handler + RequestContext tests,
  and the integration harness now sends an Origin header like a browser.

* test(identity): assert forgot-password rejects a forged Origin end-to-end

Drives the failure path through the real HTTP pipeline: a forgot-password
request carrying an Origin header outside CorsOptions.AllowedOrigins is
rejected (500) instead of returning the uniform OK, proving a spoofed
origin can never be turned into a reset link.

* test(identity): assert e-mail links resolve to the requesting front-end

Adds EmailLinkOriginTests: drives forgot-password and register through
the real pipeline and inspects the captured MailRequest body, asserting
the reset link points at the SPA origin from the request's Origin header
(:5174 vs :5173, proving per-front resolution) and that the confirmation
link targets the SPA /confirm-email page rather than the API route.

Adds the two dev SPA origins to the integration harness allow-list so
per-front resolution can be exercised.

Not yet executed locally: Windows Smart App Control blocks the freshly
rebuilt unsigned test DLLs (0x800711C7); runs in CI (Linux).

* test(identity): match confirmation/reset e-mail by subject; richer timeout

The register flow also emits a welcome e-mail (via the UserRegistered
integration event), so matching only by recipient grabbed the wrong
message. Match the confirmation e-mail by its subject, and likewise the
reset e-mail, and include the captured messages in the timeout error to
diagnose misses.

* test(identity): drop e-mail-body integration test; rely on unit coverage

The integration harness does not execute enqueued Hangfire mail jobs
(mail-asserting tests such as TenantExpiryScanJobTests invoke the job
synchronously), so the confirmation/reset e-mails never reach the
capturing mail service and EmailLinkOriginTests could not observe them.

The link content is already covered where it is built: UserPasswordServiceTests
asserts the reset link (origin + tenant + encoding) by capturing the enqueued
MailRequest, OriginResolverTests covers origin resolution, and an integration
test asserts a forged Origin is rejected. Reverts the harness allow-list
entries that only that test needed.

* docs(identity): describe origin comments by intent, not the prior behavior

* fix(identity): reword confirm-email comment to satisfy S125

The explanatory comment above the confirm-email URI build read like
commented-out code to SonarAnalyzer (S125) because of its parentheses
and trailing semicolon, failing the -warnaserror backend build. Reword
it as plain prose; behaviour is unchanged.

* refactor(identity): dedicated FrontendOptions for e-mail link origins

Address review on #1323. Replace the CorsOptions-coupled, throw-on-miss
OriginResolver with a framework-level front-end origin resolver, so any module
that builds user-facing links (Identity today; Notifications/Billing/Tickets
next) resolves them the same way.

- New FSH.Framework.Web.Frontend: FrontendOptions (AllowedOrigins + DefaultOrigin)
  + IFrontendOriginResolver/FrontendOriginResolver. Validated at startup
  (ValidateOnStart) so a deployment missing both fails loud on boot instead of
  500-ing on the first password-reset — resolves the silent CorsOptions.AllowAll
  and empty-Production-list traps.
- ResolveForCurrentRequest() (self-service: forgot-password, self-register):
  validates the Origin header against the allow-list, returns the canonical
  entry (not the client's casing), falls back to DefaultOrigin when no header is
  present (curl / Scalar / mobile / server-to-server), and throws a 400-mapped
  CustomException on a present-but-forged origin (was InvalidOperationException
  -> 500). Matching is component-wise via Uri (port exact).
- ResolveDefault() (operator-driven: register, resend-confirmation): targets the
  recipient's app via DefaultOrigin instead of the operator's Origin, so a
  tenant user provisioned from the admin app no longer gets a link into :5173.
  Also serves background jobs that have no HttpContext.
- Dedup: ApiOrigin() folded into IRequestContext.Origin (its existing contract);
  RequestContextService owns the config-first/request-host logic and
  UserProfileService reads IRequestContextService.Origin for avatar URLs.
- appsettings: FrontendOptions (dev 5173/5174 + default 5174; Production empty =
  deploy requirement). Rebased onto main (#1324 CORS allow-list).

* refactor(web): address origin-resolver review nits (log level, docs, boot message)

- Log rejected origins at Debug, not Warning: the auth endpoints are
  anonymous, so bot/forged traffic would flood the aggregator; a genuine
  deployer misconfig still surfaces as a 400 to the affected SPA's users.
- Document that FrontendOptions:DefaultOrigin is a single global (not
  per-tenant/custom-domain aware) so operator-driven links land on one SPA.
- Make the FrontendOptions startup-validation message first-run actionable,
  matching the JwtOptions "set it before starting the host" precedent.

* fix(web): require FrontendOptions:DefaultOrigin at startup

The boot validation accepted AllowedOrigins-only (DefaultOrigin empty), yet
operator-driven register/resend, every non-browser caller (no Origin header)
and background jobs resolve through DefaultOrigin. Such a host booted clean
then 500'd on the first admin register or non-browser request - the same
surprise-runtime-break the fail-loud validation was meant to prevent.

Require DefaultOrigin unconditionally; AllowedOrigins stays additive (widening
which request origins may be echoed into self-service links). Same-origin /
reverse-proxy topologies still work with DefaultOrigin alone. Fold the
redundant second AddHttpContextAccessor() call into the platform's existing one.

* fix(web): keep booting when FrontendOptions:DefaultOrigin is unset

DefaultOrigin was validated with ValidateOnStart, so an existing deployment
that upgraded without configuring it stopped booting — a setting it may never
exercise took the whole host down, and the operator's first signal was a
container that would not come up.

Fail loud at first use of the feature, not at process start:

- drop the startup validation; the host boots with DefaultOrigin unset
- ResolveDefault falls back to the API's own origin (OriginOptions:OriginUrl)
  so links land somewhere serviceable instead of going dark
- UseHeroPlatform logs one startup Warning naming the setting, the file and
  what degrades without it

The fallback is deliberately the configured API origin and never the current
request's host: ResolveDefault exists because the caller is not the recipient,
so an operator-driven confirmation link must not point at the admin app.
Forged-origin rejection is unchanged — a present-but-unlisted Origin is still
a 400, never swapped for the fallback.

* fix(web): fall back to the request host when no origin is configured at all

appsettings.Production.json ships OriginOptions:OriginUrl empty as well, so a
deployment that upgraded without touching either setting still had no origin to
build a link from and 500'd on the first operator-driven register/resend - the
exact failure the boot-safety fallback was meant to remove.

ResolveDefault now walks DefaultOrigin, then the configured API origin, then the
current request's host, and only throws when there is no request either (a
background job). The request host is the API's own, never the caller's Origin
header, so an operator-driven link still cannot point at the admin SPA.

* fix(web): resolve links against the default when no allow-list is configured

appsettings.Production.json ships FrontendOptions:AllowedOrigins empty, and
browsers attach an Origin header to the forgot-password and self-register POSTs
even same-origin. Matching a present header against an empty list returned no
canonical entry, so every legitimate password reset and self-registration came
back 400 on the shipped Production config - and on any single-SPA or
reverse-proxy deployment.

With no allow-list there is nothing to validate against, so the header is
discarded and the link resolves through the server-side default. The client's
value is never echoed, so a forged origin against a configured list is still
rejected with 400.

The startup Warning now reports an empty AllowedOrigins independently of a
missing DefaultOrigin: a deployment can configure one and not the other, and
setting only the default silently sends every user to the same front-end.

Also matches origins through IdnHost, so a list entry written in Unicode
matches the punycode form browsers actually send instead of failing closed, and
pins the handler contract on CustomException rather than the arbitrary
exception type the old test stubbed.

* fix(web): count the allow-list after normalization in the startup warning

Unparseable entries are dropped when the resolver normalizes the list, so a
list of nothing but typos matched the empty-list fallback at runtime while the
warning, reading the raw config array, saw a configured list and stayed quiet.
The operator got neither their allow-list nor a diagnostic.

The warning now counts the normalized list, and reports separately when only
some entries were dropped - those origins are rejected with 400 rather than
silently ignored.

* docs(web): stop claiming the Scalar try-it UI sends no Origin header

Scalar.AspNetCore 2.14.14 ships no default proxy URL (the option exists but
binds null, and no proxy host is baked into the assembly), so the try-it panel
fetches straight from the browser and sends the API's own origin. Listing it
alongside curl and server-to-server callers was wrong: those genuinely send no
Origin and fall back to the default, while Scalar hits the allow-list branch
and needs the API origin listed to exercise forgot-password or self-register.

* docs(rules): document the front-end origin resolver in the security rule

The rule file agents read before touching CORS, headers or rate limiting had no
entry for FrontendOptions, so the next person to add an e-mail link had nothing
telling them which resolver method matches which recipient - a choice where both
options compile and both return a plausible origin.

* docs(agents): list front-end link origins in the security rule index

The index line is how an agent decides whether to open security.md at all.

* fix(deploy): wire FrontendOptions into the docker and terraform deploys

Both shipped deployment paths left `FrontendOptions` empty, so the resolver fell
through to the API origin and every password-reset / e-mail-confirmation link
pointed at `https://api.../reset-password` and `https://api.../confirm-email` --
SPA routes that do not exist on the API. Each path already knows the SPA URLs, so
the fix is to pass them through:

- `docker-compose.yml` -- `FrontendOptions__AllowedOrigins__0/1` from the existing
  `FSH_ADMIN_URL` / `FSH_DASHBOARD_URL`, with the dashboard as `DefaultOrigin` so
  an operator-driven register / resend lands on the tenant app, not on admin.
- Terraform `app_stack` -- a `frontend_environment_variables` map mirroring the
  CORS one, built from the resolved `admin_url` / `dashboard_url` plus
  `api_extra_cors_origins` (extra SPA origins the deployer already trusts, which
  would otherwise start getting a 400 on forgot-password once the list is
  non-empty). The API domain is deliberately *not* carried over from the CORS
  list: allow-listing it reintroduces the same wrong-destination link.
  `DefaultOrigin` is the dashboard, falling back to admin, and stays empty when
  the stack hosts neither -- the pre-existing `OriginOptions__OriginUrl` behaviour.

The Docker README gains the link-building meaning of those two `.env` URLs and a
troubleshooting row for a link that lands on the API.

Verified: `docker compose config` renders the three new keys; `terraform fmt
-check -recursive` and `terraform validate` pass; the `DefaultOrigin` expression
checked in `terraform console` for all three branches (dashboard, admin-only,
neither).

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(infra): keep api_extra_cors_origins out of the e-mail-link allow-list

api_extra_cors_origins grants an origin permission to CALL the API, which is
what its description promises. Copying it into FrontendOptions:AllowedOrigins
also let those origins receive a password-reset or e-mail-confirmation URL with
the token in it, turning a CORS grant into a credential-link grant.

The two lists stay separate: CORS still includes the extra origins, e-mail links
only the SPAs this stack hosts, which arrive via admin_url/dashboard_url.

An origin listed only for CORS now gets a 400 from the anonymous self-service
endpoints, which is the intended fail-closed behaviour.

* fix(web): stop deriving e-mail-link origins from the request host

ResolveDefault() fell through DefaultOrigin -> OriginOptions:OriginUrl ->
the request's own Host header. Both fallbacks are now gone.

The Host tier is the security half. appsettings.Production.json ships
FrontendOptions:AllowedOrigins [], DefaultOrigin "", OriginUrl "" and
AllowedHosts "*", so on the shipped production config a forgot-password
POST with a forged Host header mails the reset token to the attacker's
domain. Before this resolver existed the same path threw, so this was a
regression introduced by the fallback, not a pre-existing hole.

The OriginUrl tier is the correctness half, and it is why the second
fallback goes too. These links address SPA routes (/confirm-email,
/reset-password); the API serves confirm-email under
api/v{version}/identity, so a link built on the API's own origin is a
404. "Degrade to the API origin" stopped being serviceable the moment
the paths changed.

What is left is DefaultOrigin or a 500 naming the setting, and the
startup log for a missing DefaultOrigin moves from Warning to Error to
match: the consequence is no longer degradation. Both shipped deploy
paths (docker compose, terraform) already set it; the gap is a bare
appsettings.Production.json. Upgrade note for same-origin reverse-proxy
deployments that set only OriginUrl: set FrontendOptions:DefaultOrigin
to the same value.

The eight tests that pinned the removed tiers are inverted, not deleted;
the request-host one now asserts the throw and that the attacker's host
never reaches the message. Framework.Tests 152/152, Identity.Tests
312/312, solution builds clean under TreatWarningsAsErrors.

* fix(config): keep the dev SPA origins out of Production

`appsettings.Production.json` shipped `"AllowedOrigins": []` for both CorsOptions
and FrontendOptions, on the assumption that an empty array clears the base file.
It does not: a JSON array is flattened to indexed keys, an empty one writes no
indices at all, and the binder concatenates whatever the earlier provider left.
A production deployment therefore trusted `http://localhost:5173` and `:5174` —
as a CORS origin, and as an origin that may appear inside a password-reset link.

The dev origins move to `appsettings.Development.json`, which Production never
loads, so the empty arrays in the Production file are now true.

`ShippedConfigurationTests` loads the shipped files the way the host does and
asserts what each environment actually gets. Verified by mutation: putting one
origin back in `appsettings.json` turns it red.

The CorsOptions half of this is pre-existing (`main` has the same shape) and is
fixed here because it is the same defect in the same file; without it the fix
would read as "localhost is untrusted now", which would only be half true.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* fix(frontend-origin): validate DefaultOrigin and correct three stale comments

`DefaultOrigin` was only checked for emptiness while `AllowedOrigins` went
through `Uri.TryCreate`. A value like `app.example.com` — no scheme, the usual
`.env` slip — bound cleanly, produced no startup diagnostic, and turned every
e-mail link into a relative URL no mail client makes clickable. It is now
required to parse as an absolute URI, and failing that is the same Error as
being unset.

Three comments still described the fallback chain this PR removed:

- `Web/Extensions.cs` said the resolver falls back to the API's own origin and
  logs a Warning. There is no fallback (it throws) and the log is an Error. That
  one sits in protected code, where the next maintainer would have read it as
  "safe degradation exists" and re-introduced the tier.
- `app_stack/main.tf` said an empty list leaves the API resolving links from
  `OriginOptions__OriginUrl`. That tier is gone; those flows answer 500.
- The rejection log wrote the caller-controlled `Origin` header verbatim. It is
  truncated and stripped of line breaks now, the same treatment the global
  exception handler gives the request path.

* fix(frontend-origin): treat an unusable DefaultOrigin as an unset one at run time

The startup check added in the previous commit calls a `DefaultOrigin` that is
not an absolute URL "the same failure class as an unset value", but only the log
line agreed: the resolver still handed the raw string back, so `app.example.com`
produced a relative URL in every e-mail, which no mail client makes clickable,
and nothing on the request path reported a problem.

It now goes through the same normalization the allow-list gets: a value that
does not parse as an absolute URI is dropped, and `ResolveDefault()` fails the
way it does when nothing is configured. A base path is preserved (validating must
not collapse `https://example.com/app` to its authority, or `/reset-password`
404s), and both branches are covered. Reverting the guard turns the first red.

The startup message said "is not set" for a value that is set but unusable; it
says "is not set to an absolute URL" now.

* fix(api): fail fast in Production when FrontendOptions:DefaultOrigin is unset

Without it, register / resend-confirmation / forgot-password can only return
500, so a missing or relative value now stops the API at boot, in the existing
Production fail-fast block next to the connection string and signing key. Not
validated in AddHeroPlatform because the DbMigrator also calls it and never
sends links. Non-Production keeps the single startup Error.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…d of losing the write (#1387)

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* fix(identity): reject a stale profile update instead of losing the write

`PUT /identity/profile` is a full-representation update: every field is assigned
from the request, so a caller working from a stale read blanks whatever changed
in between. Nothing on the request said which version the caller had edited, so
the server could not tell a deliberate overwrite from a lost update and accepted
both.

`AspNetUsers.ConcurrencyStamp` is already mapped as an EF concurrency token and
Identity's store rotates it on every `UserManager.UpdateAsync`, so the version
marker exists — it just was not on the wire. `GET /identity/profile` now
publishes it as a strong `ETag`, and `PUT /identity/profile` honours `If-Match`:
a token that no longer matches gets `412 Precondition Failed` instead of
silently winning. No migration and no schema change.

The header stays optional — absent means today's behaviour, so existing clients
keep working. A `ponytail:` comment marks the future path where it becomes
required and a missing header answers `428`.

Details worth calling out:

- The precondition is checked immediately after the user is loaded, before the
  storage calls. Any later and a rejected update would already have uploaded an
  orphan blob or, on the `deleteCurrentImage` path, deleted the avatar for a
  request that then fails and changes nothing in the database.
- `IdentityResult`'s `ConcurrencyFailure` is mapped to the same 412. Identity's
  store returns it rather than throwing, so a race lost one layer down used to
  surface as a generic 500.
- `RefreshSignInAsync` now runs after the success guard. It used to refresh the
  sign-in even when the update had failed.
- `*` in `If-Match` asks only that the resource exist. Weak validators can never
  satisfy the strong comparison the header mandates, so they answer 412. A
  malformed header answers 400: 412 would send a client into a refetch-and-retry
  loop it can never win, since the broken header is its own bug.

Tests: integration coverage for the ETag shape, matching/stale/list/`*`/weak/
malformed preconditions, token rotation and the avatar-survives-412 case, plus a
handler unit test that the tokens reach the service.

* test(identity): assert a rejected profile update leaves no partial write

The avatar case only checked the image URL. `SetPhoneNumberAsync` persists on its
own, ahead of the final `UserManager.UpdateAsync`, so a precondition checked too
late would let a field through on a request that then answers 412. Asserting the
name as well pins that down, and the comment now says what the test proves rather
than claiming the storage call itself is observed.

* fix(dashboard): send If-Match when saving the profile, retry once on 412

`updateMyProfile` reads the profile, merges the edited fields and PUTs the whole
representation back. Nothing tied that write to the version it was built from, so a
concurrent change — another tab, a phone, a slow save racing a fast one — was
silently overwritten.

The read now also picks up the profile's `ETag` and the PUT echoes it in `If-Match`,
so the server can answer 412 instead of accepting a stale representation. A 412 is
retried once from a fresh read: the token rotates on writes the user never thinks of
as profile edits (a password change, a failed sign-in, a new avatar), and turning
those into a failed save would be noise. A second 412 propagates.

`apiFetch` grew an `onResponse` hook, because it returns the parsed body and there
was no way to reach a response header from a caller.

Note for anyone running the API on a separate origin (the dev setup does — the page
is on 5174 and the API on 7030): `ETag` is not a CORS-safelisted response header, so
the browser hides it from JS unless the API also sends
`Access-Control-Expose-Headers: ETag`, and `If-Match` has to be an allowed request
header. The framework's CORS policy does neither today, which is a separate change
in protected code. Until it lands this path degrades to the old behaviour — the
client reads no tag and sends no precondition. Same-origin deployments (the shipped
`apiBase: ""` default) are unaffected.

* test(identity): gate the ETag CORS exposure the front-end depends on

The dashboard specs mock `Access-Control-Expose-Headers: ETag`, which the API does
not send: `FSH.Framework.Web.Cors` never calls `WithExposedHeaders`. A browser
therefore hides the tag from JS on any cross-origin call, the client stops sending
`If-Match`, and the endpoint silently falls back to the lost-update behaviour this
branch set out to fix -- with every test still green.

Assert it instead of describing it in a comment. The test is skipped so the suite
stays green until the framework change lands (protected code, needs approval);
the skip reason names exactly what has to change to un-skip it.

Verified: un-skipped it fails on the missing header; with `WithExposedHeaders("ETag")`
added locally to the AllowAll branch it passes. That temporary edit was reverted --
`src/BuildingBlocks` is untouched by this branch.

Refs #1359

* feat(cors): expose ETag and allow If-Match so clients can use preconditions

`ETag` is not a CORS-safelisted response header, so a browser hid it from JS on every
cross-origin call -- which is every dev run, since both React apps point `apiBase` at
the API's own origin. A front-end that cannot read the validator cannot send `If-Match`,
so the optimistic-concurrency precondition on `PUT /identity/profile` degraded straight
back to the lost update it exists to prevent, with the whole suite still green.

Exposed for both policy branches: neither `AllowAnyHeader` nor `WithHeaders` implies
exposure, and the header carries no data of its own, only a validator.

`if-match` joins `AllowedHeaders` in both shipped appsettings for the mirror-image reason:
with `AllowAll: false` the request header is stripped before it reaches the endpoint.

Gates: `CorsPolicyTests` covers both branches at the policy level and
`GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead` covers it end to end,
so the front-end mocks can no longer hide a server that stops sending the header. Verified by
mutation -- dropping the argument turns all three red; restored and re-run green.

Refs #1359

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(dashboard): take the profile ETag from the read the form was seeded with

The save read the profile again and used that read's ETag as If-Match. A
tag fetched at save time is current by construction, so it matched
whatever a concurrent writer had just stored and the PUT went through:
the endpoint gained 412 handling while the client could never trigger
it. The lost update the PR set out to stop happens between the user
seeing the values and pressing save, and nothing was watching that gap.

The ETag now travels with the profile the form was seeded from, held in
a ref so a background refetch cannot advance it to a version the user
never saw. The 412 retry is gone with it: the only body available is the
one typed against the old values, so resending it against a fresh tag
performs exactly the overwrite the 412 prevented. The page warns, keeps
the typed edits on screen, adopts the current version, and waits for a
deliberate second save.

Two consequences fell out of getting there. The refetch after a conflict
has to pass staleTime 0, or the client's 30s default hands back the
cached copy carrying the tag the server just rejected. And Save is now
disabled until the profile read lands, since a save carries that read's
unedited fields and version — previously the save built its own body, so
it could run without one.

The topbar and the security page share this query key, so they read
through the same ETag-carrying function: one key, one shape.

Gates: the two new specs fail on the previous client (the save sent the
post-change tag; the retry overwrote) and pass after. profile.spec 9/9,
tsc and lint clean. Full dashboard suite 151/153 with 2 failures that
pass on their own run and touch none of this — a pre-existing flake
under 6 workers, reported separately.

* fix(identity): survive a weak ETag and a rotated stamp on the profile form

Three defects an independent review found in the concurrency work, all of them
on paths a normal user walks.

**A compressing edge makes the profile permanently unsavable.** The endpoint only
emits a strong validator, but Cloudflare (and any edge that re-encodes a
response) downgrades the tag it forwards to `W/"..."` by default. The client
stored and echoed that verbatim, the server dropped it under the strong
comparison `If-Match` mandates, and every save answered 412 — on a profile
nobody else was touching, with the UI blaming a concurrent editor. The client
strips the `W/` prefix: a weak tag can only be a transport artefact here.

**Changing the avatar guaranteed a 412 on the next save.** Setting the image is a
second write to the same row, so Identity rotates the concurrency stamp, but the
image mutation only invalidated the query — the form kept the pre-image tag.
It adopts the new version instead, which also refreshes the cached copy the
topbar avatar reads.

**A lost race on the delete-avatar path could destroy the blob.** The `If-Match`
guard is not the last word: another writer can still land between it and
`UpdateAsync`, which then fails with `ConcurrencyFailure` and maps to 412. The
old blob had already been removed by then, leaving `AspNetUsers.ImageUrl`
pointing at something that no longer exists — unrecoverable, and invisible until
the next page load. The delete now runs only after the database write succeeds.

Both client fixes are mutation-checked: reverting either turns its new spec red.
The third has no automated test — reproducing it needs a real race between
`FindByIdAsync` and `UpdateAsync` inside one request — so it is inspection plus
the existing 17 profile integration tests staying green.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* fix(identity): close three gaps review found around the precondition

- `onSuccess` fired `adoptCurrentVersion()` without awaiting it, so `isPending`
  dropped before the new tag was in hand: the button re-enabled over a spent tag
  and a quick second save 412'd against the user's own write, with a toast
  blaming someone else. A failed refetch was also an unhandled rejection that
  left the form stranded on a tag the server had already rejected.
- `if-match` in `CorsOptions:AllowedHeaders` had no gate. `CorsPolicyTests`
  builds its configuration in memory, so removing the header from the shipped
  appsettings kept the suite green while the restricted policy stripped the
  precondition off every PUT — the feature would degrade back to the lost update
  it exists to prevent, silently. The new test loads the shipped files the way
  the host does, for both environments.
- The disabled-save path (profile read failing) was described in the PR body as
  one of the two latent bugs fixed, and had no test. It has one now.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants